Allow relative urls for fastboot and ember-data #128
Conversation
RobbieTheWagner
changed the title
RobbieTheWagner
changed the title
RobbieTheWagner
changed the title
| return owner && owner.lookup('service:fastboot'); | ||
| }), | ||
|
|
||
| protocol: computed(function() { |
There was a problem hiding this comment.
why use CPs with dependent keys? Can these be set in init?
There was a problem hiding this comment.
Can we use Fastboot service here? https://github.com/ember-fastboot/ember-cli-fastboot/tree/master#fastboot-service
There was a problem hiding this comment.
For verifying if Fastboot is used isn't this the preferred way to do it in an addon—to avoid an error if the consumer doesn't have Fastboot installed?
One thing to note is that someone may use your addon in an app that doesn't have FastBoot installed. In that case, if your addon tries to inject the fastboot service, they'll get an exception saying the fastboot service cannot be found.
Instead, you can write a computed property that uses the low-level getOwner functionality to lookup the fastboot service directly.
https://ember-fastboot.com/docs/addon-author-guide#accessing-the-fastboot-service
There was a problem hiding this comment.
@chrism Yea you're absolutely right, thansk for pointing out!
|
@nlfurniss I copied this stuff from https://github.com/tchak/ember-fetch-adapter/blob/master/addon/index.js It allows users of this mixin to make requests when in fastboot. Without these changes, you get an error with node-fetch complaining about |
|
I have been speaking with @tchak about this. Might be good for him to weigh in here 😃 |
|
@nlfurniss @tchak any updates on this? |
|
I've been getting this error too when using with Fastboot after following the instructions.
It would be great to see this merged. |
|
@nlfurniss @rwjblue what do we need to do to get this merged? |
|
Can you rebase and resolve the conflicts? Also what about tests? |
|
@nlfurniss yes, I can rebase, but tests would need a very special setup to force fastboot and ember-data, and I am not sure it would be something we could easily do. If I rebase, can we merge as is? This has been open quite awhile. |
| return owner && owner.lookup('service:fastboot'); | ||
| }), | ||
|
|
||
| protocol: computed(function() { |
There was a problem hiding this comment.
Can we use Fastboot service here? https://github.com/ember-fastboot/ember-cli-fastboot/tree/master#fastboot-service
| * @param url | ||
| * @returns {*} | ||
| */ | ||
| buildServerURL(url) { |
There was a problem hiding this comment.
buildServerURL sounds Fastboot specific, maybe change the method name or move the non-fastboot branch to _fetchRequest? This function can also be unit tested.
| return `${protocol}${url}`; | ||
| } else if (!httpRegex.test(url)) { | ||
| try { | ||
| return `${protocol}//${host}${url}`; |
@xg-wang I just copied all this, as mentioned previously. Since I know it works currently, and it's copied from this other addon, I am not sure what the best way to proceed is. |
|
Close by #143 |
Copied over the
buildServerUrlsfunction from ember-fetch-adapter to allow relative urls with fastboot and ember-data.